-
Notifications
You must be signed in to change notification settings - Fork 145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace old APIs with explicit addTransceivers #50
base: develop-pre-2.3.0
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes themselves look good, but my concern is around browser compatibility.
addTransceiver
is not as well supported (FF 59, Chrome 69, Edge 79) as createAnswer
is (FF 22, Chrome 51, Edge 15). What can we do about this? Does adaptor.js handle this?
Also, another comment: in the README there is a section demonstrating creating an offer/answer. Please update that part too. |
That's a good point. AFAIK, adapter.js doesn't handle this. I'll spend some time to see if there's a good solution for this. I think this should be a common issue. |
Looking here: https://webrtchacks.github.io/adapter/adapter-latest.js I do see addTransceiver here, is there anything else we need or is this ready to merge now, @MixMasterMitch ? |
As of today, addTransceiver is supported by all except opera, and createAnswer is supported by all. |
I tried this locally.. it causes the ICE candidate pairs to not get printed correctly (tested on Chrome). I tried the regular P2P and also WebRTC ingestion, both with the same result. Current JS test page:
Changes from this PR:
Looks like the way the candidate pair gets logged needs to change as well to go with this approach. |
Issue #49
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.